Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PostCapture screen implementation with in memory Image storage #32

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yasith
Copy link
Member

@yasith yasith commented Aug 23, 2023

TODOs for later

  • Use image orientation from the transform data of ImageProxy
  • Scale image to fit screen width/height
  • Overlay buttons for saving/discarding
  • Async listening for image to be ready (might be needed for lower end devices)

TODOs for later
- Use image orientation from the transform data of ImageProxy
- Scale image to fit screen width/height
- Overlay buttons for saving/discarding
- Async listening for image to be ready (might be needed for lower end devices)
@yasith yasith requested a review from Kimblebee August 23, 2023 16:09
/**
* Used to store a single [Image].
*/
interface ImageCache {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: You can replace the set/get with a var

interface ImageCache {
    var image: Bitmap?
}

class ImageCacheImpl : ImageCache {
    override var image: Bitmap?
        get() = TODO("Not yet implemented")
        set(value) {}
}

Copy link
Collaborator

@jsaund jsaund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it makes it cleaner / simpler to rely on a image loading library to manage all this for you.
So if you're using Coil for example, you will want to take the JPEG byte array and save that somewhere
Then in you're Composable you can use AsyncImage and in it's ImageRequest use .data(jpegByteArray)

You can then tie to a custom fetcher (I got this from coil-kt/coil#171 (comment))

class ByteArrayFetcher : Fetcher<ByteArray> {

    override fun key(data: ByteArray): String? = null

    override suspend fun fetch(
        pool: BitmapPool,
        data: ByteArray,
        size: Size,
        options: Options
    ): FetchResult {
        return SourceResult(
            source = ByteArrayInputStream(data).source().buffer(),
            mimeType = null,
            dataSource = DataSource.MEMORY
        )
    }
}

Register the new fetcher -- add to ComponentRegistry when creating the ImageLoader

Now you should get more efficient bitmap pool management as well, resizing, rotation handling

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't currently navigate back after a successful image capture

)
}
composable(SettingsRoute) { SettingsScreen(navController = navController) }
composable(PostCaptureRoute) { PostCaptureScreen(
onBackPressed = { navController.navigateUp() }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we'd only be navigating to this page from within the app, we can just use .popBackStack()
they behave the same, except when using navigateUp() will return to the other app that brought you to a page. wont have to worry about that here.

Regardless, right now we can't use either without the app crashing (after rebinding use cases), i found that to be the case when navigating back to preview from the settings page. A quick fix for this is to just use navController.navigate(PreviewRoute), though the bug causing this issue should still be addressed; i think we have already touched on it.

@Kimblebee
Copy link
Collaborator

There are some issues with orientation of the image captured. front-facing camera is mirrored, and the rear-facing camera is upside-down

implementation("androidx.hilt:hilt-navigation-compose:1.0.0")

// Compose - Testing
androidTestImplementation "androidx.compose.ui:ui-test-junit4:$compose_version"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where does compose_version come from? Other packages just use:
androidTestImplementation("androidx.compose.ui:ui-test-junit4")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants